Fix silent / misleading failures in paginated waterdata pagination by thodson-usgs · Pull Request #273 · DOI-USGS/dataretrieval-python

thodson-usgs · 2026-05-08T21:44:36Z

Summary

Found while reviewing the repo for high-value bugfixes. Two latent bugs in _walk_pages and the parallel pagination loop in get_stats_data (dataretrieval/waterdata/utils.py) cause silent or misleading failures whenever a paginated waterdata request is interrupted mid-walk.

Bug 1 — `resp` is stale when `client.request()` itself raises

while curr_url:
    try:
        resp = client.request(...)              # ← may raise ConnectionError, Timeout, …
        dfs.append(_get_resp_data(resp, geopd))
        curr_url = _next_req_url(resp)
    except Exception:
        error_text = _error_body(resp)          # ← uses *previous* page's resp
        logger.error("Request incomplete. %s", error_text)
        ...

When the network call raises before resp is reassigned, resp still references the previous successful page. The log message thus describes the wrong response, and _error_body() falls into resp.json() on a 200 OK body that wasn't an error envelope — which can itself raise and bury the original failure.

Bug 2 — no status-code check on mid-pagination responses

The initial request gate at line 615 reads if resp.status_code != 200: raise RuntimeError(_error_body(resp)). The pagination loop does not. So a 5xx response with a JSON error body that lacks numberReturned flows into _get_resp_data(), which silently returns an empty DataFrame; _next_req_url() finds no next link; and the loop exits with no error logged anywhere. The caller gets truncated data and never knows.

This affects every paginated waterdata getter: get_daily, get_continuous, get_monitoring_locations, get_time_series_metadata, get_combined_metadata, get_field_measurements, get_field_measurements_metadata, get_peaks, get_channel, get_reference_table, plus the stats getters (get_stats_por, get_stats_date_range).

Fix

Two-line change to each loop:

if resp.status_code != 200:
    raise RuntimeError(_error_body(resp))

…immediately after client.request(...) (mirrors the initial-request gate). And:

except Exception as e:
    logger.error("Request incomplete: %s", e)

…instead of _error_body(resp), so the actual exception text surfaces — works for both the network-error case (uses the exception) and the 5xx case (uses the RuntimeError(_error_body(resp)) we just raised on a fresh resp).

The "best-effort" behavior is preserved: on failure the loop still logs and returns whatever pages were collected. The change is purely to make the failure observable and the log message accurate.

Test plan

test_walk_pages_logs_actual_exception_when_request_raises — simulates a ConnectionError mid-pagination, asserts the logged error reflects the actual exception ("boom") rather than something derived from a stale prior-page response.
test_walk_pages_surfaces_5xx_mid_pagination — page 2 returns HTTP 503 with a JSON body, asserts the 503 / ServiceUnavailable shows up in the error log. Previously this was silently swallowed.
Existing test_walk_pages_multiple_mocked (happy path) still passes.
Full mocked test suite: 278 passed, ruff + format clean.

🤖 Generated with Claude Code

Two bugs in _walk_pages (and the parallel get_stats_data pagination loop) caused silent or misleading failures when a paginated request was interrupted mid-walk: 1. The except handler called _error_body(resp) — but when client.request() itself raised (ConnectionError, Timeout, etc.), `resp` still pointed at the *previous successful page*. The "incomplete" log message therefore described the wrong request, and on a non-JSON 200 body would itself raise inside resp.json() and bury the original failure. 2. No status-code check was performed on paginated responses. The initial request guards `if resp.status_code != 200`, but the loop doesn't. A 5xx body that didn't include "numberReturned" was silently turned into an empty DataFrame by _get_resp_data, the `next` link wasn't found, and the loop quietly exited — handing the user truncated data with no error logged anywhere. This affects every paginated waterdata getter (get_daily, get_continuous, get_monitoring_locations, …). Fix: - Guard the loop body with `if resp.status_code != 200: raise RuntimeError(_error_body(resp))`, mirroring the initial request. - Capture the exception with `as e` and log `e` instead of touching the stale `resp`. Same change in get_stats_data. Behavior preserved: on failure the loop still logs and returns whatever pages were collected ("best effort"). The change is purely to make the failure observable and the log message accurate. Two new tests cover (a) network-exception mid-pagination and (b) 5xx mid-pagination, asserting that the actual failure surfaces in the error log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The status-code guard appeared four times in waterdata/utils.py (initial-request and pagination paths in both _walk_pages and get_stats_data) with bit-identical bodies. Extract into a single named helper; the helper's docstring carries the "silent-empty-frame" WHY that the inline comment was apologizing for. Also: - trim the second multi-line comment in _walk_pages to a single line noting the actual constraint (`resp` may be stale) - hoist `import logging` to module level in waterdata_utils_test.py - condense the two new test docstrings to one-line contract statements; the bug history lives in the commit message Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Follow-up cleanups on top of the silent-truncation fix: - New tests for the get_stats_data parallel pagination loop, mirroring the _walk_pages ConnectionError + 5xx pair (closes coverage gap from the prior commits). - Lift the hard-coded logger-name string to a module-level _LOGGER_NAME constant derived from the utils module's __name__ (self-syncing). - logger.error → logger.exception in both pagination except clauses so the traceback is automatically attached to the log record. - _raise_for_non_200 docstring: drop the historical bug narration, add a one-line note distinguishing it from Response.raise_for_status (we route through _error_body for USGS-API-aware messages). - Drop the inconsistent "resp may be stale" comment; the `as e` and variable naming already convey intent, and get_stats_data already sidesteps it by logging `url` instead of `resp.url`. - _run_get_stats_data_with_failure: make monkeypatch a required arg (both callers pass it), and swap the lambda for MagicMock so future signature drift in _handle_stats_nesting isn't silently masked. - Tighten test docstrings that narrated the PR/bug. - Extract _error_log_messages(caplog) helper (used 4 times). - Lift the conditional links ternary out of the dict literal in _resp_ok. - NEWS.md: bump date to today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes two failure modes in waterdata pagination where mid-walk interruptions could either (a) log misleading/stale error details when requests.Session.request() raises, or (b) silently truncate results when a later page returns a non-200 response.

Changes:

Added a shared _raise_for_non_200() helper and applied it to both the initial request and subsequent pagination requests in _walk_pages and get_stats_data.
Updated pagination exception handling to log the actual exception message rather than deriving an error from a potentially stale prior response.
Added regression tests for mid-pagination request exceptions and mid-pagination non-200 responses; documented the fixes in NEWS.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`dataretrieval/waterdata/utils.py`	Ensures every paginated page is status-checked and avoids stale-response error logging during pagination failures.
`tests/waterdata_utils_test.py`	Adds targeted regression tests covering mid-pagination request exceptions and 5xx responses.
`NEWS.md`	Documents the pagination bugfixes and their user-visible impact.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ation-errors # Conflicts: # NEWS.md

…ion warning Copilot noted that the get_stats_data pagination-failure warning logs only the base statistics URL (constant across all pages), dropping the next_token cursor that uniquely identifies which page in the sequence failed. The original `resp.url` reference was abandoned because `resp` may be stale when client.request() itself raises — but next_token is the loop variable and is always current. Add `(next_token=<value>)` to the warning, plus a focused test that asserts the cursor surfaces in the WARNING-level log entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot PR DOI-USGS#273 review: the variable name `page2_500` is misleading because the mocked response is a 503 (ServiceUnavailable), not a 500. Renamed for clarity so future readers debugging a failure don't have to mentally reconcile the name against the actual status code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

dataretrieval/waterdata/utils.py:1152

Same as in _walk_pages: the PR description mentions switching to logger.error(...), but this code uses logger.exception(...) and will log a full traceback for mid-pagination failures. Please confirm that this additional verbosity is intended; otherwise align the logging call with the described behavior.

            except Exception as e:  # noqa: BLE001
                logger.exception("Request incomplete: %s", e)
                logger.warning(
                    "Request failed for URL: %s (next_token=%s). "

…per docstring Two small follow-ups from Copilot's review of 9c6481f: - Revert `logger.exception` -> `logger.error` in both `_walk_pages` and `get_stats_data` except blocks. The PR description always advertised `logger.error("Request incomplete: %s", e)`; the simplify pass had switched to `logger.exception(...)` (which auto-attaches a traceback) but mid-pagination failures are expected, best-effort cases — the exception text is already in the message via `%s`, and a per-page traceback would be noisy. Reverting matches the PR description and the original intent. - Drop the literal count from `_error_log_messages`'s docstring. The prior wording ("the four pagination-failure tests below") was already stale-prone, and Copilot flagged it. Replaced with a count-free phrasing so future test additions don't render it misleading. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot PR DOI-USGS#273 review: the docstring of `_error_log_messages` wrapped mid-compound-word as `pagination-\n failure`, which renders as "pagination- failure" in help output. Rephrased so the term stays on one line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@v6

Simplify-review consensus across three reviewers (reuse / quality / efficiency) on the conftest.py from the previous commit: - Drop the `pytest.mark.live` marker double-application: no `-m live` selector is wired anywhere in CI; only `pytest.mark.flaky` carries behavior. The `live` marker plus its `pytest_configure` registration was dead metadata. - Tighten `_TRANSIENT_PATTERNS`: drop `r"timed out"` (subsumed by the `Timeout` arm) and `r"Bad Gateway|Service Unavailable|Gateway Timeout"` (subsumed by the `RuntimeError:\s*5\d\d:` prefix from `_raise_for_non_200`). Three load-bearing patterns left. - Drop defensive `getattr(item, "fixturenames", ())` — pytest items reliably expose `fixturenames`; the default-tuple fallback is dead defense. - Shorten the module docstring: keep the WHY paragraph (PR DOI-USGS#273 → surfaced errors → CI flake risk) and drop the WHAT bullets that narrate the code below. - Add a one-line justification for the 5-second backoff (USGS upstream typically recovers from brief 5xx within that window). - Tighten the NEWS.md entry: drop conftest implementation detail (registers a marker, applies via collection hook, …) and keep the user-relevant claim. Skipped from the review (reviewer can decide): - Inverting the auto-mark heuristic to opt-in `@pytest.mark.live` would force decorating ~35 test functions; the current "everything not using requests_mock is live" is zero-cost on success and self-extending for new tests. - Centralizing the transient-error regex set in `dataretrieval/ waterdata/utils.py` would couple library code to test infrastructure for one pattern. Separately, bump GitHub Actions to Node-24-compatible versions (@v6 across all three workflows) ahead of the deprecation warning surfaced on the previous CI run: actions/checkout@v4 → v6, actions/setup-python@v5 → v6. Node 20 is deprecated and will be removed from runners on 2026-09-16; the default flips on 2026-06-02. NEWS.md entry updated to mention the bump. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

PR DOI-USGS#273 fixed pagination loops in dataretrieval.waterdata.utils specifically; other modules (wqp, samples, nadp, streamstats, nldi, nwis) reach the network through different paths and don't share that regression. Narrow _PUBLIC_GETTERS to waterdata's 16 user-facing getters and add a tests/waterdata file-path scope check, which also prevents name-collisions with legacy nwis getters (get_ratings, get_daily) from spuriously marking nwis tests. After this change, the retry marker is applied to 55 tests across tests/waterdata_test.py, tests/waterdata_nearest_test.py, tests/waterdata_filters_test.py, and tests/waterdata_ratings_test.py, with zero matches outside that directory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

#273 surfaced a transient HTTP 502 from upstream on its merge-to-main CI run that would have been silently swallowed by _walk_pages and turned into an empty DataFrame. The status-code-aware behavior is correct for users, but it makes every test that hits the live USGS Water Data API susceptible to flaking on a transient blip. This PR adds: - pytest-rerunfailures to the `test` optional dependency set. - tests/conftest.py that (a) registers a `live` pytest marker; (b) auto-applies it to every test that does not take `requests_mock` as a fixture (the existing mock-driven convention in this repo); and (c) configures live-marked tests to retry up to twice on a 5-second backoff — but ONLY when the failure trace matches one of a narrow set of transient-upstream patterns: `429:` / `5xx:` prefixes from `_raise_for_non_200`, `ConnectionError` shapes, and timeout strings from the requests/urllib3 stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

`_walk_pages` and `get_stats_data`'s pagination loops have, since PR DOI-USGS#273, logged failures correctly but preserved a "best effort" contract of returning whatever pages had been collected when a follow-up page failed. The waterdata API exposes no resume cursor once a paginated walk is interrupted, so the partial DataFrame couldn't reliably be extended — silently returning it handed callers truncated data they had no way to know was truncated. Both loops now wrap any mid-pagination exception (429, 5xx, network error) in a ``RuntimeError`` carrying: - the number of pages successfully collected, - the upstream cause (as both the message text and ``__cause__`` for programmatic inspection), - a short menu of recovery actions (wait and retry, reduce request size, or obtain an API token). The shared helper ``_paginated_failure_message`` builds the user- facing string so both loops stay aligned. Behavior change: callers that previously consumed partial DataFrames on transient upstream blips will now see an exception they need to handle (typically: retry, possibly with a smaller ``limit`` or narrower query). Called out in NEWS. Tests: - Replaced the prior best-effort-preserves-partial assertions with raises-with-cause-chain assertions for all three failure modes (connection error, 5xx, 429), in both ``_walk_pages`` and ``get_stats_data`` variants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

) `_walk_pages` and `get_stats_data`'s pagination loops have, since PR #273, logged failures correctly but preserved a "best effort" contract of returning whatever pages had been collected when a follow-up page failed. The waterdata API exposes no resume cursor once a paginated walk is interrupted, so the partial DataFrame couldn't reliably be extended — silently returning it handed callers truncated data they had no way to know was truncated. Both loops now wrap any mid-pagination exception (429, 5xx, network error) in a ``RuntimeError`` carrying: - the number of pages successfully collected, - the upstream cause (as both the message text and ``__cause__`` for programmatic inspection), - a short menu of recovery actions (wait and retry, reduce request size, or obtain an API token). The shared helper ``_paginated_failure_message`` builds the user- facing string so both loops stay aligned. Behavior change: callers that previously consumed partial DataFrames on transient upstream blips will now see an exception they need to handle (typically: retry, possibly with a smaller ``limit`` or narrower query). Called out in NEWS. Tests: - Replaced the prior best-effort-preserves-partial assertions with raises-with-cause-chain assertions for all three failure modes (connection error, 5xx, 429), in both ``_walk_pages`` and ``get_stats_data`` variants. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…r helpers, clarify docs Three review responses bundled together: - chunking.py module docstring: define ``k`` as the candidate filter chunk count before using it in the planner description. - ``QuotaExhausted`` docstring: drop the stale "silently truncate" framing. PR DOI-USGS#273 / DOI-USGS#279 already raise on a mid-pagination 429, so this exception is the structured-recovery alternative (partial frames in hand) rather than a defense against silent truncation. - Move chunker-only orphans from filters.py to chunking.py: ``_WATERDATA_URL_BYTE_LIMIT`` (the URL byte ceiling), ``_FetchOnce`` TypeVar, ``_combine_chunk_frames``, and ``_combine_chunk_responses``. filters.py was a leftover home from the pre-unification two-decorator stack; these helpers have no callers outside the chunker. Test ``test_multi_value_chunked_lazy_url_limit`` now monkeypatches the constant on its new module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

thodson-usgs and others added 2 commits May 8, 2026 16:44

thodson-usgs requested a review from Copilot May 15, 2026 02:06

Copilot started reviewing on behalf of thodson-usgs May 15, 2026 02:06 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread dataretrieval/waterdata/utils.py Outdated

thodson-usgs mentioned this pull request May 15, 2026

Add multi-value GET-parameter chunker for waterdata OGC API #276

Closed

7 tasks

thodson-usgs and others added 2 commits May 14, 2026 21:13

Merge remote-tracking branch 'upstream/main' into fix-paginated-trunc…

44c434b

…ation-errors # Conflicts: # NEWS.md

thodson-usgs requested a review from Copilot May 15, 2026 02:17

Copilot started reviewing on behalf of thodson-usgs May 15, 2026 02:17 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread tests/waterdata_utils_test.py Outdated

thodson-usgs requested a review from Copilot May 15, 2026 02:44

Copilot started reviewing on behalf of thodson-usgs May 15, 2026 02:45 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread tests/waterdata_utils_test.py Outdated

Comment thread dataretrieval/waterdata/utils.py

thodson-usgs requested a review from Copilot May 15, 2026 13:44

Copilot started reviewing on behalf of thodson-usgs May 15, 2026 13:44 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread tests/waterdata_utils_test.py Outdated

thodson-usgs requested a review from Copilot May 15, 2026 14:02

Copilot started reviewing on behalf of thodson-usgs May 15, 2026 14:02 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

thodson-usgs marked this pull request as ready for review May 15, 2026 14:31

thodson-usgs merged commit 0a14ba5 into DOI-USGS:main May 15, 2026
12 checks passed

thodson-usgs mentioned this pull request May 15, 2026

Auto-retry live-API tests on transient upstream errors #277

Merged

4 tasks

This was referenced May 16, 2026

WIP: perf(waterdata): Optional parallel chunk processing in multi_value_chunked (draft) #278

Draft

fix(waterdata): Raise on mid-pagination failures instead of silently truncating #279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix silent / misleading failures in paginated waterdata pagination#273

Fix silent / misleading failures in paginated waterdata pagination#273
thodson-usgs merged 8 commits into
DOI-USGS:mainfrom
thodson-usgs:fix-paginated-truncation-errors

thodson-usgs commented May 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

thodson-usgs commented May 8, 2026

Summary

Bug 1 — resp is stale when client.request() itself raises

Bug 2 — no status-code check on mid-pagination responses

Fix

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Bug 1 — `resp` is stale when `client.request()` itself raises